Bugfix: Server-initiated room disconnect shutdown fix by alan-george-lk · Pull Request #205 · livekit/client-sdk-cpp

alan-george-lk · 2026-07-13T17:42:48Z

Summary

Fixes server-initiated room disconnect shutdown by routing explicit disconnect, destructor cleanup, server disconnect events, and EOS through a shared shutdown() helper, which by proxy cleans up event code
Ensure server Disconnected events remove the C++ FFI listener, stop subscriptions, clear room-owned state, and fire onDisconnected exactly once without re-sending an FFI disconnect

Before fix, when server deleted the room:

[2026-07-13 10:02:31.515] [livekit] [error] FfiClient listener threw: mutex lock failed: Invalid argument

Above observed on Mac, but crashed on Linux (user-reported). No longer after the fix.

Testing

Original issue reproduced via local binary (not committed), then fixed and confirmed no longer occurred
Add regression coverage for synthetic FFI server disconnects and local-SFU room deletion via lk --dev room delete
Consolidates emitFfiEvent into standalone header helper ffi_utils.h for use across two tests now

Ticket

BOT-464

Co-authored-by: Cursor <cursoragent@cursor.com>

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 1 additional finding.

xianshijing-lk

lgtm, assuming you address those comments.

xianshijing-lk · 2026-07-14T03:04:10Z

+    const bool has_room_state = connection_state_ != ConnectionState::Disconnected || listener_id_ != 0 ||
+                                room_handle_ || local_participant_ || !remote_participants_.empty();
+    if (teardown_started_ || !has_room_state) {
      return false;


should we return true if nothing to do or it is being torndown ?

I'm open to changing this, right now our public API for disconnect() states:

@returns true if the graceful disconnect succeeds; false if the room was already disconnected (no-op) or the graceful disconnect fails.

Which this matches, false == noop. I get how that could be confusing, but I think the idea is true is when the disconnect actually happened

That is fair. We can keep the current behavior to avoid breaking changes.

FWIW we have a public connectionState so apps can check the state before trying to disconnect

…gfix-room-shutdown

Align the shared cleanup helper with LocalParticipant/FfiClient naming, keep disconnect() returning false for no-ops per its documented contract, and cover Room reuse after server-initiated shutdown. Co-authored-by: Cursor <cursoragent@cursor.com>

xianshijing-lk · 2026-07-14T04:09:14Z

lgtm, thanks

stephen-derosa · 2026-07-14T16:00:26Z

-          }
-
-          // old_* state is destroyed here when going out of scope
+          (void)shutdown(false, DisconnectReason::Unknown, false);


is EOS not a DisconnectReason?

TODO: Confirm the flow is this on server disconnect:

Server disconnect event (do shutdown)

EOS (don't need shutdown)

Verify that the EOS is double-calling shutdown right now as written (after server close)

stephen-derosa

nice lgtm!

…gfix-room-shutdown

devin-ai-integration

Devin Review found 1 new potential issue.

View 5 additional findings in Devin Review.

devin-ai-integration · 2026-07-15T19:44:36Z

+          bool should_notify = false;
          {
            const std::scoped_lock<std::mutex> guard(lock_);
-            already_disconnected = (connection_state_ == ConnectionState::Disconnected);
-            connection_state_ = ConnectionState::Disconnected;
-          }
-          if (already_disconnected) {
-            break;
-          }
-          DisconnectedEvent ev;
-          ev.reason = toDisconnectReason(re.disconnected().reason());
-          if (delegate_snapshot) {
+            // Local shutdown marks the state before awaiting the FFI response
+            // and notifies the delegate itself. Suppress that duplicate while
+            // passing server-initiated disconnects through unchanged.
+            should_notify = connection_state_ != ConnectionState::Disconnected;
+          }
+          if (should_notify && delegate_snapshot) {
+            DisconnectedEvent ev;
+            ev.reason = toDisconnectReason(re.disconnected().reason());
            delegate_snapshot->onDisconnected(*this, ev);
          }
          break;


🔴 Server disconnect notification can fire twice when a user disconnect races with a server disconnect event

The disconnect notification is decided (should_notify = true at src/room.cpp:1202) without updating the room's connection state, so a concurrent user-initiated disconnect on another thread can also pass its own state check and fire the same notification, resulting in the delegate receiving two disconnect callbacks instead of one.

Impact: The application's disconnect handler runs twice with different reasons, which can cause double-cleanup, incorrect state transitions, or crashes in user code that assumes exactly-one semantics.

Race window between kDisconnected handler and user disconnect()

The old code at the base commit always set connection_state_ = Disconnected inside the kDisconnected handler under the lock. This created mutual exclusion: whichever path (FFI event or user disconnect()) set the state first would prevent the other from firing the delegate.

The new code at src/room.cpp:1196-1209 only reads the state but never writes it:

bool should_notify = false; { const std::scoped_lock<std::mutex> guard(lock_); should_notify = connection_state_ != ConnectionState::Disconnected; // ← state is NOT updated here } if (should_notify && delegate_snapshot) { delegate_snapshot->onDisconnected(*this, ev); }

Race sequence:

FFI thread: kDisconnected handler acquires lock, reads Connected, sets should_notify = true, releases lock.

User thread: disconnect() → shutdown() acquires lock, also reads Connected (unchanged!), moves out all state, sets Disconnected, releases lock, calls delegate->onDisconnected(ClientInitiated).

FFI thread: resumes and calls delegate->onDisconnected(RoomDeleted) because should_notify was already captured as true.

Result: delegate receives two onDisconnected calls. The integration test UserDisconnect explicitly asserts delegate.count.load() == 1, so this race can cause test failures and violates the documented contract.

Prompt for agents

In src/room.cpp, the kDisconnected event handler (around line 1196-1209) reads connection_state_ under the lock to decide whether to notify the delegate, but does not update the state. This creates a race window where a concurrent user-initiated disconnect() on another thread can also pass its state check and fire the delegate, resulting in two onDisconnected calls. The fix should restore the old behavior of setting connection_state_ = ConnectionState::Disconnected inside the kDisconnected handler's locked section, matching what the base commit did. This ensures mutual exclusion: whichever path (the FFI kDisconnected event or the user's disconnect() call to shutdown()) sets the state first will prevent the other from firing the delegate. Specifically, in the kDisconnected case block, after computing should_notify, add: connection_state_ = ConnectionState::Disconnected; inside the locked section. This way shutdown() on another thread will see the state as already Disconnected and return false (no-op), preventing the double notification.

Was this helpful? React with 👍 or 👎 to provide feedback.

alan-george-lk added 4 commits July 13, 2026 10:17

Initial impl of shutdown repro

8f5433c

Use room name for repro

d5dda8c

Implementation of teardown feature for bugfix

0b9ea06

Add integration test

9effbcb

alan-george-lk changed the title ~~Bugfix: server-initiated room disconnect teardown fix~~ Bugfix: Server-initiated room disconnect teardown fix Jul 13, 2026

alan-george-lk and others added 3 commits July 13, 2026 12:35

Remove standalone disconnect repro from branch

738693b

Co-authored-by: Cursor <cursoragent@cursor.com>

Cleanup

0672226

Unify emitFfiEvent into standalone header

7810fa3

alan-george-lk marked this pull request as ready for review July 13, 2026 22:36

alan-george-lk requested review from stephen-derosa and xianshijing-lk as code owners July 13, 2026 22:36

devin-ai-integration Bot reviewed Jul 13, 2026

View reviewed changes

xianshijing-lk approved these changes Jul 14, 2026

View reviewed changes

alan-george-lk and others added 2 commits July 13, 2026 21:53

Merge branch 'main' of github.com:livekit/client-sdk-cpp into alan/bu…

9f56644

…gfix-room-shutdown

alan-george-lk changed the title ~~Bugfix: Server-initiated room disconnect teardown fix~~ Bugfix: Server-initiated room disconnect shutdown fix Jul 14, 2026

This comment was marked as resolved.

Sign in to view

Cleanup weird test

a2e6a9e

This comment was marked as resolved.

Sign in to view

stephen-derosa reviewed Jul 14, 2026

View reviewed changes

stephen-derosa approved these changes Jul 14, 2026

View reviewed changes

alan-george-lk force-pushed the alan/bugfix-room-shutdown branch from 3cbddb6 to a2e6a9e Compare July 14, 2026 19:12

alan-george-lk added 3 commits July 14, 2026 18:58

Merge branch 'main' of github.com:livekit/client-sdk-cpp into alan/bu…

e8569e3

…gfix-room-shutdown

Removing shutdown from EOS event

7534b22

Address unrecoverable case

55299c3

This comment was marked as resolved.

Sign in to view

Now test against rust fix for EOS

8416551

devin-ai-integration Bot reviewed Jul 15, 2026

View reviewed changes

Cleaner log lines

a6a8d20

Uh oh!

Conversation

alan-george-lk commented Jul 13, 2026 • edited by stephen-derosa Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Ticket

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

xianshijing-lk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xianshijing-lk Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

alan-george-lk Jul 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xianshijing-lk Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

stephen-derosa Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

xianshijing-lk commented Jul 14, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

stephen-derosa Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

alan-george-lk Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

stephen-derosa left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alan-george-lk commented Jul 13, 2026 •

edited by stephen-derosa

Loading

alan-george-lk Jul 14, 2026 •

edited

Loading